fix: breaking changes are now recognized as meaningful #140
Conversation
| @@ -39,12 +39,11 @@ | |||
| "*": "prettier --ignore-unknown --write" | |||
| }, | |||
| "dependencies": { | |||
| "@pkgjs/parseargs": "^0.11.0", | |||
| "conventional-commits-parser": "^5.0.0" | |||
There was a problem hiding this comment.
This jalopy does not recognize the feat!: message form of commits.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 63.55% 67.22% +3.67%
==========================================
Files 6 6
Lines 107 119 +12
Branches 12 14 +2
==========================================
+ Hits 68 80 +12
Misses 39 39 ☔ View full report in Codecov by Sentry. |
| return "meaningful"; | ||
| } | ||
|
|
||
| if (alwaysIgnoredTypes.has(type)) { | ||
| return { type }; | ||
| } | ||
| } catch { | ||
| /* empty */ |
There was a problem hiding this comment.
[Refactor] I'm not a big fan of try/catch as a control flow concept. It's not type-safe, is hard to mentally model, and can accidentally wrap around unintentionally throwing code too. Let's restrict what's being wrapped to just the toConventionalChangelogFormat call.
| import { | ||
| parser, | ||
| toConventionalChangelogFormat, | ||
| } from "@conventional-commits/parser"; |
There was a problem hiding this comment.
[Refactor] 🤔 @conventional-commits/parser hasn't had a commit or sustained GitHub issue triage in 3 years, and uses throw as a form of control flow. Whereas conventional-commits-parser released 3 months ago. I'm a little averse to going down to an older, less-maintained package if it's avoidable.
What do you think about instead using a small regex to check for a subject, optionally an area/scope, and ! at the beginning? /^\w+(\(\w+\))?!:/ or something like that?
Maybe: also filing an issue on conventional-commits-parser to add support for !?
| ["docs: bump\n\nBREAKING CHANGE: breaks things", "meaningful"], | ||
| ["docs: bump\n\nBREAKING-CHANGE: breaks things", "meaningful"], | ||
| ["style: bump\n\nBREAKING CHANGE: breaks things", "meaningful"], | ||
| ["style: bump\n\nBREAKING-CHANGE: breaks things", "meaningful"], |
There was a problem hiding this comment.
[Testing] Thinking about test edge cases...
!in the commit body (so not a breaking/meaningful change)- Footer note other than
BREAKING CHANGE - Multiple footer notes
| @@ -40,6 +40,17 @@ describe("getCommitMeaning", () => { | |||
| ["chore(deps): release", "release"], | |||
| ["chore(deps): release 1.2.3", "release"], | |||
| ["chore(deps): release v1.2.3", "release"], | |||
| // Breaking changes are meaningful no matter the type of the commit. | |||
There was a problem hiding this comment.
[Style] Just nitpicking here, I think the comment is explanatory from the test cases.
| // Breaking changes are meaningful no matter the type of the commit. |
JoshuaKGoldberg
added
the
status: waiting for author
|
This is all great feedback! Thanks for taking the time to go over it. I'm going to step away from this PR though instead of finishing it up.
|
Closes #139.